fix implicit 64 to 32 bit conversion warnings #3955

antond-weta · 2023-08-23T02:47:54Z

Description

Enabling the implicit 64 to 32 bit conversion warnings shows several hundreds of them. This is my attempt to fix them all.

Tests

I have built the code on an Apple silicon Mac (both command line and Xcode), and an x64 linux machine, and have run the tests. I have not tested on Windows, or any 32-bit hardware.

I'm not familiar with the project enough to say that my testing was adequate. Any help would be appreciated.

Checklist:

I have read the contribution guidelines.
I have updated the documentation, if applicable.
I have ensured that the change is tested somewhere in the testsuite
(adding new test cases if necessary).
If I added or modified a C++ API call, I have also amended the
corresponding Python bindings (and if altering ImageBufAlgo functions, also
exposed the new functionality as oiiotool options).
My code follows the prevailing code style of this project. If I haven't
already run clang-format before submitting, I definitely will look at the CI
test that runs clang-format and fix anything that it highlights as being
nonconforming.

Signed-off-by: Anton Dukhovnikov <[email protected]>

lgritz · 2023-08-23T03:19:36Z

src/cineon.imageio/libcineon/EndianSwap.h

@@ -86,15 +86,15 @@ inline char SwapBytes( char& value )


 template <typename T>
-void SwapBuffer(T *buf, unsigned int len)
+void SwapBuffer(T *buf, size_t len)


I don't understand what's going on here. The parameter was unsigned int, and the loop variable below is also unsigned int. Why change to size_t? And if the param is size_t, doesn't it just make a new potential problem or warning in the loop below that still is unsigned int?

SwapBuffer gets called from EndianSwapImageBuffer, passing the length parameter. EndianSwapImageBuffer in turn is called from multiple places with size_t as the length parameter. Changing the param type seemed more logical to me. I have indeed overlooked the int in the loop.

Signed-off-by: Anton Dukhovnikov <[email protected]>

jessey-git

Meta comment / battle for another day: What style of casts are preferred? Functional int(x) or c-style (int)x -- both are used in this PR, sometimes even within the same file.

I tried to look over things carefully but will want to take another fresh look tomorrow just in case.

jessey-git · 2023-08-23T03:52:20Z

src/hdr.imageio/hdrinput.cpp

@@ -48,7 +48,7 @@ class HdrInput final : public ImageInput {
    bool read_native_scanline(int subimage, int miplevel, int y, int z,
                              void* data) override;
    bool close() override;
-    int current_subimage(void) const override { return m_subimage; }
+    int current_subimage(void) const override { return (int)m_subimage; }


Unintentional change? m_subimage is already an int?

jessey-git · 2023-08-23T04:19:39Z

src/libOpenImageIO/deepdata.cpp

@@ -91,7 +91,7 @@ class DeepData::Impl {  // holds all the nontrivial stuff
            spin_lock lock(m_mutex);
            if (!m_allocated) {
                // m_cumcapacity.resize (npixels);
-                size_t totalcapacity = 0;
+                int totalcapacity = 0;


Seems odd to use int here for a total when the items added are each uint32_t themselves.

jessey-git · 2023-08-23T04:40:58Z

src/tiff.imageio/tiffinput.cpp

-    imagesize_t tile_pixels = m_spec.tile_pixels();
-    imagesize_t nvals       = tile_pixels * m_inputchannels;
+    int tile_pixels = (int)m_spec.tile_pixels();
+    int nvals       = tile_pixels * m_inputchannels;


I believe large tiles would be broken with this. tile_pixels * m_inputchannels can overflow 2billion here e.g. a 24k, 4-channel image, with 1 tile

These 2 values get passed to palette_to_rgb and bit_convert which take ints. If int overflows for large tiles, the issue is already there. We may want to modify those two to take a larger type.

This is actually a good example why this work is worth doing.

Although each dimension of an image (width, height, depth, nchannels, subimages, etc) are usually declared as int for simplicity (and the fact that at this point, we don't imagine any of those being > 2^31), we use imagesize_t (a.k.a. uint64_t) when we have to talk about the number of pixels or values in a tile or images (because those other dimensions may multiply).

So I think in this case, you want to keep tile_pixels and nvals as imagesize_t. It's the count parameters in palette_to_rgb and bit_convert that should probably should be upgraded to imagesize_t rather than int.

jessey-git · 2023-08-23T04:49:06Z

src/fits.imageio/fits_pvt.h

@@ -28,7 +28,7 @@ namespace fits_pvt {
 // struct in which we store information about one subimage. This information
 // allow us to set up pointer at the beginning of given subimage
 struct Subimage {
-    int number;
+    size_t number;


Why the change here? Subimages in OIIO are ints and other formats store their equivalent as an int. Actually is this even used? I see one line that sets this value but nothing ever reads from it; maybe remove it entirely.

jessey-git · 2023-08-23T05:32:00Z

src/libutil/paramlist_test.cpp

@@ -21,7 +21,7 @@ static std::string
 test_numeric(T* data, int num_elements, TypeDesc type)
 {
    ParamValue p("name", type, num_elements, data);
-    int n = type.numelements() * num_elements;
+    int n = (int)type.numelements() * num_elements;


This is opposite of what was done inside src/libutil/paramlist.cpp where size_t was kept.

jessey-git · 2023-08-23T06:16:57Z

src/tiff.imageio/tiffoutput.cpp

@@ -1243,7 +1243,7 @@ TIFFOutput::write_scanline(int y, int z, TypeDesc format, const void* data,
    std::vector<unsigned char> cmyk;
    if (m_photometric == PHOTOMETRIC_SEPARATED && m_convert_rgb_to_cmyk)
        data = convert_to_cmyk(spec().width, data, cmyk);
-    size_t scanline_vals = size_t(spec().width) * m_outputchans;
+    int scanline_vals = spec().width * m_outputchans;


While this won't overflow because TIFF has a width restriction of 1048576, a calculation a few lines below this keeps the size_t for a similar multiplication against the format size.

Yeah, this is another one that definitely should stay size_t (or even more explicitly be imagesize_t, though those are the same thing on 64 bit platforms).

lgritz · 2023-08-23T18:49:56Z

I'm worried that this is a rathole that's just going to waste a lot of Anton's time and will touch lots of things that have been working for a long time, possibly introducing subtle bugs if we aren't very careful.

Before continuing to fix these individually, let's step back and think about a few things:

Why is this happening now? I feel like we've fixed this kind of warning before, any time the compilers detect them. Is xcode different? Is it doing the equivalent of -Wall by default? Is Apple silicon different because the definitions of long or long long have changed versus x86_64 or something of that nature?
Are the same warnings revealed on other platforms if we use -Wall? (This can be enabled from from the OIIO cmake build with -DEXTRA_WARNINGS=ON.)
Anton, before trying to actually fix any more of these, can you go back to the original code, build it (maybe with -DSTOP_ON_WARNING=OFF so it doesn't stop at the first one) and just paste the full warning report here or in an issue? Then let's just spend a little time looking it over and discussing what should really be fixed and how, what is a "false positive", and what should have pragmas to ignore the errors (and how local can we make those pragmas -- per file or even per line might be more targeted than turning the warnings off for the whole codebase).

antond-weta · 2023-08-23T21:54:41Z

Larry, doing Wall does not find anything new for me, the log is clean. The -DEXTRA_WARNINGS=ON does find a lot of stuff, like unused comments.

Xcode enables shorten-64-to-32 by default, which is, I assume, is not covered by Wall. I can reproduce the same behaviour by adding the option to the cmake config, see attached. Oddly, Xcode disables sign-compare by default. Both are worth having enabled in my opinion.

Wshorten-64-to-32.log.zip

lgritz · 2023-08-23T22:22:01Z

Sorry, my bad. We always enable -Wall. What I meant is -Wextra, which is turned on by the EXTRA_WARNINGS cmake variable. I guess "all" isn't really "all"!

Here's what I think we should do:

Let's take our hands off of this jumbled PR for the moment.
If EXTRA_WARNINGS doesn't catch all of these (i.e., if -Wextra doesn't implicitly include shorten-64-to-32, then let's add -Wshorten-64-to-32 to the clause here so that we have a way to enable it easily in the build, optionally.
Let's then attack the warnings bit by bit, not all in one PR, but go one area or issue at a time of some related warnings (such as, just the ones in the TIFF plugin as one batch) so we can get consensus on the right way to solve it. For some of them, such as ones that touch public header files and may affect downstream projects, we should proceed very cautiously.

antond-weta · 2023-08-23T22:35:41Z

Sounds good.

Larry, while we are looking at warnings, could you check this one. Shouldn't there be logical and, not bitwise? Am I missing something?
https://github.com/OpenImageIO/oiio/blob/56c734a6752d2a03af3455c50967e1f7a3835d92/src/libtexture/texturesys.cpp#L2349-L2358

lgritz · 2023-08-23T22:51:39Z

That one is intentionally bitwise, for performance reasons. (&& is short-circuited, i.e. there is an implied branch.) The pragma immediately before that is to disable the warning because we really do want this the way we wrote it.

lgritz · 2023-08-23T22:57:03Z

By the way, a lot of what you did on this PR is just fine. Part of asking to step back and turn them into smaller PRs is to cleave off the good ones from the ones that require some more thought. For example, all the ones of this nature:

    int numImg = m_images.size();

changing to size_t, those are fine and good to change since obviously vector::size() returns size_t and so it's weird that we ever assigned that to an int.

fix implicit 64 to 32 bit conversion warnings

e14236e

Signed-off-by: Anton Dukhovnikov <[email protected]>

lgritz reviewed Aug 23, 2023

View reviewed changes

antond-weta added 3 commits August 23, 2023 16:33

disabling the warnings on windows for now

f5a7858

Signed-off-by: Anton Dukhovnikov <[email protected]>

fix crash

332c48e

Signed-off-by: Anton Dukhovnikov <[email protected]>

another fix

d7187ae

Signed-off-by: Anton Dukhovnikov <[email protected]>

jessey-git reviewed Aug 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix implicit 64 to 32 bit conversion warnings #3955

fix implicit 64 to 32 bit conversion warnings #3955

antond-weta commented Aug 23, 2023

lgritz Aug 23, 2023

antond-weta Aug 23, 2023 •

edited

Loading

jessey-git left a comment

jessey-git Aug 23, 2023

jessey-git Aug 23, 2023

jessey-git Aug 23, 2023

antond-weta Aug 23, 2023

lgritz Aug 23, 2023

jessey-git Aug 23, 2023

jessey-git Aug 23, 2023

jessey-git Aug 23, 2023

lgritz Aug 23, 2023

lgritz commented Aug 23, 2023

antond-weta commented Aug 23, 2023

lgritz commented Aug 23, 2023

antond-weta commented Aug 23, 2023

lgritz commented Aug 23, 2023

lgritz commented Aug 23, 2023

fix implicit 64 to 32 bit conversion warnings #3955

Are you sure you want to change the base?

fix implicit 64 to 32 bit conversion warnings #3955

Conversation

antond-weta commented Aug 23, 2023

Description

Tests

Checklist:

Choose a reason for hiding this comment

antond-weta Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

jessey-git left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgritz commented Aug 23, 2023

antond-weta commented Aug 23, 2023

lgritz commented Aug 23, 2023

antond-weta commented Aug 23, 2023

lgritz commented Aug 23, 2023

lgritz commented Aug 23, 2023

antond-weta Aug 23, 2023 •

edited

Loading